Configuration: refactor network configuration, migrate to chainId, remove unnecessary connectivity polling #1485

sohkai · 2020-07-10T09:53:46Z

Recommended to review commit by commit—each commit is meant to be a separate PR, but due to changes stacking from one commit to each other, and the possible conflicts that may arise from one PR to another, I've decided to keep all the changes in a single PR.

At the moment, this PR has not been thoroughly tested! Since so many of the changes are built on top of each other, my plan is to do a thorough round of testing after a few more sets of changes (coming soon).

Changes:

a4f33cd, 5468dba: Refactors the network configuration
- Updates some of its fields to be more descriptive
- Updates some of the copy related to network naming to be aligned on The Ethereum network, The xDai network, The Ethereum Rinkeby test network, etc.
- Updates some internal function naming
5d24e1f, 47e9d2a: Account module simplifications
- As we don't connect automatically to a network (see Account: remember last connected account #1363), users can never be connected to the wrong network at the moment
584456c: Rename main to ethereum
- "Mainnet" is not precise as a network selector
c456ea8: Updates Network Preferences to be based on chain ID
4f20948: Replace network.type with network.chainId (fixes Remove network.type #1472)
d507385: Refactor available networks for onboarding
851e9b6, 9c19599: Remove unused connectivity polling

Note: we are not able to completely eliminate network.type yet, as there are some APIs from aragonUI that need to be updated (see https://github.com/aragon/aragon-ui/pull/788).

… network names

sohkai · 2020-07-10T11:42:11Z

src/components/AccountModule/AccountModule.js

@@ -189,7 +184,7 @@ function AccountModule() {
                providerInfo={providerInfo}
                walletConnectionStatus={walletConnectionStatus}
                walletListening={walletListening}
-                walletOnline={walletListening}
+                walletOnline={walletOnline}


Not sure, but I think this should be a fix 😄

Whoops! Haha I can definitely tell you this is a fix. 😆

sohkai · 2020-07-10T11:42:53Z

src/components/AccountModule/connection-hooks.js

@@ -71,8 +41,7 @@ export function useWalletConnectionDetails(
    isWalletAndClientSynced

  const defaultOkConnectionDetails = {
-    connectionMessage: `Connected to ${walletNetworkName}`,
-    connectionMessageLong: `Connected to Ethereum ${walletNetworkName} Network`,


We weren't using this key as far as I could tell

Perhaps the previous implementation wasn't being applied correctly but I do think there does need to be a distinction between a long and short message.

Previously

This change

The previous version with the button length being shorter than the pop over is a tighter presentation choice imo.

👍 to this; we definitely want shorter messages for the short connection message.

sohkai · 2020-07-10T11:45:35Z

src/network-config.js

@@ -32,17 +37,20 @@ export const networkConfigs = {
      portisDappId ? { id: 'portis', conf: portisDappId } : null,
    ].filter(p => p),
  },


I thought the above renamings were more clear:

endpoints communicates that this is a network-specific URL much better than nodes.defaultEth

environment communicates that this is what will be available in the environment.js file much better than settings does (it's not really network settings anymore)

…n ID

…atus

sohkai · 2020-07-11T10:09:27Z

package.json

@@ -105,14 +104,14 @@
    "bundlewatch": "bundlewatch",
    "start": "node scripts/start",
    "start:local": "node scripts/launch-local",
-    "start:mainnet": "cross-env ARAGON_ETH_NETWORK_TYPE=main npm start",
+    "start:mainnet": "cross-env ARAGON_ETH_NETWORK_TYPE=ethereum npm start",


I've started renaming our internal references to "main" or "mainnet" to "Ethereum", as "mainnet" is not descriptive enough (e.g. xDai has a mainnet, Aragon Chain will have a mainnet).

sohkai · 2020-07-11T10:10:14Z

src/App.js

@@ -45,18 +45,6 @@ const INITIAL_DAO_STATE = {
  repos: [],
 }

-const SELECTOR_NETWORKS = [


This felt like an odd place to configure the available networks during onboarding, so I've moved it out into the /onboarding folder as static configuration.

sohkai · 2020-07-11T10:11:06Z

src/App.js

-  componentDidMount() {
-    // Only the default, because the app can work without the wallet
-    pollConnectivity([web3Providers.default], connected => {
-      this.setState({ connected })


As far as I could tell, this connected state is actually not used anymore now that we do the block polling in the Account module.

Good riddance—this was spiking our API requests to ETH nodes!

sohkai · 2020-07-11T10:12:03Z

src/apps/Organization/Organization.js

-                  ? `select the ${sanitizeNetworkType(network.type)} network`
-                  : 'unlock your account'
-              } in ${getProviderString(
+              {`Please unlock your account in ${getProviderString(


Similar to the Account module, I've simplified places where we check the connect account's network vs. the expected network as the user currently can never have their account enabled on a different network.

sohkai · 2020-07-11T10:15:10Z

src/components/SignerPanel/ValidateWalletWeb3.js

@@ -29,28 +27,15 @@ function ValidateWalletWeb3({
    )
  }

-  if (isTransaction && walletNetworkType !== networkType) {


Similar to above, this state is now impossible to be in

sohkai · 2020-07-11T10:16:25Z

src/contexts/ActivityContext.js

-  })
+const getStoredList = (daoAddress, account) =>
+  new StoredList(
+    `activity:chainId-${network.chainId}:${daoAddress}:${account}`,


Note that this key change will reset any saved activities a user may have held.

In particular, I found it better to use an address here rather than the domain—the same organization can hold multiple (or no) ENS domains.

sohkai · 2020-07-11T10:16:54Z

src/contexts/FavoriteDaosContext.js

@@ -7,7 +7,7 @@ import { addressesEqual } from '../web3-utils'

 const FavoriteDaosContext = React.createContext()

-const storedList = new StoredList(`favorite-daos:${network.type}`)
+const storedList = new StoredList(`favorite-daos:chainId-${network.chainId}`)


Similar to above, this will reset any saved favourited DAOs.

sohkai · 2020-07-11T10:18:14Z

src/index.js

@@ -17,7 +17,6 @@ import { RoutingProvider } from './routing'
 import { ConsoleVisibleProvider } from './apps/Console/useConsole'
 import initializeSentryIfEnabled from './sentry'
 import { HelpScoutProvider } from './components/HelpScoutBeacon/useHelpScout'
-import { ClientBlockNumberProvider } from './components/AccountModule/useClientBlockNumber'


As far as I could tell, there were no uses of this provider's value, as we only show the wallet's sync block (and each connectivity check directly calculates the block drift between the wallet and the read provider).

sohkai · 2020-07-11T10:19:22Z

src/local-settings.js

@@ -100,12 +100,17 @@ export function getLocalChainId() {
  return getLocalSetting(LOCAL_CHAIN_ID) || 1337


I've taken the liberty of renaming some of the exposed methods to be more clear, without changing the underlying environment variables.

At a future point in time (e.g. as we evaluate snowpack / etc), we can make breaking changes :).

sohkai · 2020-07-11T10:21:09Z

src/network-config.js

+export function getNetworkConfig(id) {
+  const networkConfig = networkConfigurations[id]
+  if (!networkConfig) {
+    throw new Error(`Unsupported network '${id}'`)


Note that I've opted to entirely remove the unsupported network configuration.

It's impossible to statically know what chain id to expect in that case, and it's likely better the app just blows up entirely than pretend things are OK.

sohkai · 2020-07-11T10:22:31Z

src/prop-types.js

@@ -266,7 +266,6 @@ export const WalletType = PropTypes.shape({
  enable: PropTypes.bool.isRequired,
  connected: PropTypes.bool.isRequired,
  isContract: PropTypes.bool.isRequired,
-  networkType: PropTypes.string.isRequired,
  providerInfo: PropTypes.object.isRequired,
  web3: PropTypes.object.isRequired,
 })


This type doesn't seem to be used anywhere, but is perhaps still useful for the sake of documentation?

Anyhow, I've updated it with our wallet.js changes.

sohkai · 2020-07-11T10:22:59Z

src/templates/company/index.js

@@ -120,12 +119,7 @@ export default {
    const adjustedDuration = new BN(duration).toString()
    const votingSettings = [adjustedSupport, adjustedQuorum, adjustedDuration]

-    // Rinkeby has its gas limit capped at 7M, so some larger 6.5M+ transactions are


Things have changed a lot since Sept. 2019; Rinkeby now has a 10M gas limit.

sohkai · 2020-07-11T10:24:52Z

src/testnet/airdrop.js

@@ -1,12 +1,61 @@
-import tokens from '@aragon/templates-tokens'


I've removed this import and just inlined the contents so we can control them better.

Particularly, the import used the network's name rather than chain ID (and to be honest, that package is not particularly helpful).

sohkai · 2020-07-11T10:26:56Z

src/web3-utils.js

@@ -145,13 +146,18 @@ const gasPriceApi = 'https://ethgasstation.info/json/ethgasAPI.json'
 export async function getGasPrice({
  mainnet: { safeMinimum = '3', disableEstimate } = {},
 } = {}) {
-  if (network.type !== 'main') {
-    // Hardcode 10 for non-mainnet networks
+  if (network.chainId === CHAIN_IDS.XDAI) {


There does not appear to be a public API providing gas price estimates on xDai yet... which is probably fine given there are almost no transactions.

andy-hook

Really solid PR! lots of good changes in here 🚀 🔥 . I've left some comments on areas which stuck out, the main one being how we are accessing endpoints and making sure it's working as expected.

andy-hook · 2020-07-13T09:34:17Z

src/testnet/airdrop.js

-  const ts = tokens[network]
-  const depositer = new web3.eth.Contract(tokens.depositerABI, ts.depositer)
+export default (web3, financeAddr, from) => {
+  if (!testTokensEnabled) {


Suggested change

if (!testTokensEnabled) {

if (!testTokensEnabled()) {

Think we need to call this.

andy-hook · 2020-07-13T09:52:27Z

src/components/GlobalPreferences/Network/Network.js

@@ -124,37 +120,40 @@ Network.propTypes = {

 const useNetwork = wrapper => {
  const [networkError, setNetworkError] = useState(null)
-  const [ethNode, setEthNodeValue] = useState(defaultEthNode)
+  const [ethEndpoint, setEthEndpointValue] = useState(network.endpoints.read)


I'm not sure this is correct, looks like network is an alias for networkConfig.environment which doesn't contain the endpoints.

andy-hook · 2020-07-13T10:05:19Z

src/network-config.js

+    endpoints: {
+      read: providedDefaultEthNode || 'wss://mainnet.eth.aragon.network/ws',


Much prefer this naming! ✨ wonder if eth is more suitable than read?

AFAIK read's used as we separate our "read" provider for fetching info, from our write provider (the wallet provider). I actually think this is more clear than eth. 😃

andy-hook · 2020-07-13T10:26:21Z

src/chain-ids.js

+const PROTECTED_CHAIN_IDS = new Proxy(CHAIN_IDS, {
+  get(target, property) {
+    if (property in target) {
+      return target[property]
+    } else {
+      throw new Error(`Chain ID '${property}' not supported`)
+    }
+  },
+})


andy-hook · 2020-07-13T10:29:36Z

src/known-organizations/index.js

-  rinkeby: new Map(
+
+  // Ethereum Rinkeby
+  [CHAIN_IDS.ETHEREUM]: new Map(


Suggested change

[CHAIN_IDS.ETHEREUM]: new Map(

[CHAIN_IDS.RINKEBY]: new Map(

andy-hook · 2020-07-13T10:37:29Z

src/templates/dandelion/config/helpers/getBlockTime.js

 ])
+const UNKNOWN_NETWORK_BLOCK_TIME = 2


Just curious – Is this still necessary considering the previous change to unsupported network ?

andy-hook · 2020-07-13T10:40:36Z

src/App.js

-  componentDidMount() {
-    // Only the default, because the app can work without the wallet
-    pollConnectivity([web3Providers.default], connected => {
-      this.setState({ connected })


andy-hook · 2020-07-13T10:50:28Z

src/components/AccountModule/connection-hooks.js

@@ -71,8 +41,7 @@ export function useWalletConnectionDetails(
    isWalletAndClientSynced

  const defaultOkConnectionDetails = {
-    connectionMessage: `Connected to ${walletNetworkName}`,
-    connectionMessageLong: `Connected to Ethereum ${walletNetworkName} Network`,


Perhaps the previous implementation wasn't being applied correctly but I do think there does need to be a distinction between a long and short message.

Previously

This change

The previous version with the button length being shorter than the pop over is a tighter presentation choice imo.

Evalir

Boom! Quite the changes but all of these things are super clear now. :) Left some comments:

Evalir · 2020-07-14T22:32:30Z

src/network-config.js

      chainId: 1,
-      name: 'Mainnet',
+      name: 'Ethereum',


I dig this new naming a lot; but maybe it can be clearer that this is the Ethereum mainnet network?

Evalir · 2020-07-14T22:33:36Z

src/components/AccountModule/AccountModule.js

@@ -189,7 +184,7 @@ function AccountModule() {
                providerInfo={providerInfo}
                walletConnectionStatus={walletConnectionStatus}
                walletListening={walletListening}
-                walletOnline={walletListening}
+                walletOnline={walletOnline}


Whoops! Haha I can definitely tell you this is a fix. 😆

Evalir · 2020-07-14T22:47:00Z

src/components/AccountModule/connection-hooks.js

@@ -71,8 +41,7 @@ export function useWalletConnectionDetails(
    isWalletAndClientSynced

  const defaultOkConnectionDetails = {
-    connectionMessage: `Connected to ${walletNetworkName}`,
-    connectionMessageLong: `Connected to Ethereum ${walletNetworkName} Network`,


👍 to this; we definitely want shorter messages for the short connection message.

Evalir · 2020-07-14T22:49:15Z

src/chain-ids.js

+const PROTECTED_CHAIN_IDS = new Proxy(CHAIN_IDS, {
+  get(target, property) {
+    if (property in target) {
+      return target[property]
+    } else {
+      throw new Error(`Chain ID '${property}' not supported`)
+    }
+  },
+})


Evalir · 2020-07-14T22:51:51Z

src/components/SignerPanel/SignerPanel.js

@@ -360,12 +352,10 @@ class SignerPanel extends React.PureComponent {
                    >
                      <Screen>
                        <ValidateWalletWeb3
+                          hasWallet={Boolean(walletWeb3)}


I definitely don't remember exactly from where the account and other use-wallet related props are being drilled from, but we could definitely pass the connected prop corresponding to the wallet from use-wallet.

Evalir · 2020-07-14T22:53:45Z

src/testnet/airdrop.js

+}
+
+export function testTokensEnabled() {
+  return !!AIRDROP_TOKENS[network.chainId]


I think we can make this nicer by applying Boolean(). :)

stale · 2020-12-24T11:23:32Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for contributing to Aragon! 🦅

sohkai added 2 commits July 10, 2020 11:47

Configuration: refactor network configuration and align strings using…

a4f33cd

… network names

Account: remove unused code for detecting network mismatch

5d24e1f

sohkai requested a review from bpierre July 10, 2020 09:53

probot-autolabeler bot added app: organization module: account module: preferences labels Jul 10, 2020

sohkai commented Jul 10, 2020

View reviewed changes

Network: update configuration to use 'ethereum' as id of main network

584456c

vercel bot deployed to Preview July 10, 2020 12:20 View deployment

sohkai added 2 commits July 10, 2020 14:25

Environment: update function names for eth endpoint configuration

5468dba

Preferences: update connection settings' copy and base checks on chai…

c456ea8

…n ID

vercel bot deployed to Preview July 10, 2020 12:31 View deployment

sohkai added 5 commits July 10, 2020 23:49

Network: replace network.type with network.chainId

4f20948

Onboarding: move selectable networks into a constant

d507385

Network: remove top-level polling for client provider's connection st…

851e9b6

…atus

Account: remove unused client block number polling

9c19599

Account: refactor connection info rendering

47e9d2a

vercel bot deployed to Preview July 10, 2020 21:51 View deployment

sohkai commented Jul 11, 2020

View reviewed changes

sohkai marked this pull request as ready for review July 11, 2020 10:31

sohkai changed the title ~~Configuration: refactor network configuration, migrate to chainId~~ Configuration: refactor network configuration, migrate to chainId, remove unnecessary connectivity polling Jul 11, 2020

sohkai requested review from Evalir and andy-hook July 11, 2020 10:34

andy-hook reviewed Jul 13, 2020

View reviewed changes

Evalir reviewed Jul 14, 2020

View reviewed changes

bpierre mentioned this pull request Aug 3, 2020

Scaffold: 1 – Initial set-up aragon/network-dashboard#1

Merged

sohkai mentioned this pull request Aug 7, 2020

Add simple network config with Connect provider aragon/network-dashboard#8

Merged

stale bot added the abandoned label Aug 14, 2020

aragon deleted a comment from stale bot Aug 14, 2020

stale bot removed the abandoned label Aug 14, 2020

stale bot added the abandoned label Sep 13, 2020

aragon deleted a comment from stale bot Sep 13, 2020

stale bot removed the abandoned label Sep 13, 2020

stale bot added the abandoned label Oct 15, 2020

aragon deleted a comment from stale bot Oct 15, 2020

stale bot closed this Oct 23, 2020

sohkai reopened this Oct 23, 2020

stale bot removed the abandoned label Oct 23, 2020

stale bot added the abandoned label Nov 22, 2020

aragon deleted a comment from stale bot Nov 23, 2020

stale bot removed the abandoned label Nov 23, 2020

stale bot added the abandoned label Dec 24, 2020

stale bot closed this Dec 31, 2020

		@@ -100,12 +100,17 @@ export function getLocalChainId() {
		return getLocalSetting(LOCAL_CHAIN_ID) \|\| 1337

		@@ -1,12 +1,61 @@
		import tokens from '@aragon/templates-tokens'

		endpoints: {
		read: providedDefaultEthNode \|\| 'wss://mainnet.eth.aragon.network/ws',

Configuration: refactor network configuration, migrate to chainId, remove unnecessary connectivity polling #1485

Configuration: refactor network configuration, migrate to chainId, remove unnecessary connectivity polling #1485

Conversation

sohkai commented Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-hook Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-hook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-hook Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Evalir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Dec 24, 2020

sohkai commented Jul 10, 2020 •

edited

Loading

andy-hook Jul 13, 2020 •

edited

Loading

sohkai Jul 11, 2020 •

edited

Loading

andy-hook Jul 13, 2020 •

edited

Loading